Conversation
calmilamsy
left a comment
There was a problem hiding this comment.
Mostly surface-level concerns. The implementation is otherwise very good, and I'm more than happy to merge this once my issues are addressed.
src/test/java/net/modificationstation/sltest/gui/hud/TestHudText.java
Outdated
Show resolved
Hide resolved
...in/java/net/modificationstation/stationapi/impl/client/gui/hud/StationFlatteningHudText.java
Outdated
Show resolved
Hide resolved
.../main/java/net/modificationstation/stationapi/impl/client/gui/hud/StationVanillaHudText.java
Outdated
Show resolved
Hide resolved
...main/java/net/modificationstation/stationapi/impl/client/gui/hud/StationRendererHudText.java
Outdated
Show resolved
Hide resolved
src/test/java/net/modificationstation/sltest/gui/hud/TestHudText.java
Outdated
Show resolved
Hide resolved
...0/src/main/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextLine.java
Outdated
Show resolved
Hide resolved
...ain/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextRenderEvent.java
Show resolved
Hide resolved
.../main/java/net/modificationstation/stationapi/impl/client/gui/hud/StationVanillaHudText.java
Show resolved
Hide resolved
...ain/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextRenderEvent.java
Show resolved
Hide resolved
...0/src/main/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextLine.java
Show resolved
Hide resolved
mineLdiver
left a comment
There was a problem hiding this comment.
Surface level review, but I have some big concerns:
HudTextLinebeing instantiated dozens of times each frame.- Vanilla debug renderer being completely skipped.
In my opinion, HudTextLine should instead contain a template string and an array of callbacks for formatting, so that it can be instantiated once and always be up-to-date.
For example: Template "Used memory: %d\% (%dMB) of %dMB", the array of callbacks being:
var callbacks = new Supplier[] {
() -> (Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()) * 100L / Runtime.getRuntime().maxMemory(),
() -> (Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()) / 1024L / 1024L,
() -> Runtime.getRuntime().maxMemory() / 1024L / 1024L
}This is just a working example, this could be optimized by using a single class with its methods registered as callbacks, so the calculated values could be shared.
Making HudTextLine use callbacks also enables the debug HUD to just be built once instead of each frame.
As for the vanilla lines being completely canceled, I feel like a more surgical approach must be taken where there's a way to cancel each line (maybe by using something like EnumSet<VanillaHudLine> canceled?) and then overwrite the freed space while building the debug HUD. StAPI shouldn't have to render Minecraft's debug if it's unchanged.
|
My own two cents, StAPI replacing the vanilla debug hud is perfectly fine, IMO. It keeps things simple on the impl side and overall makes it easier to maintain. Concerns of other mods affecting the vanilla hud are honestly a nil point to me due to fact we can't predict what lines will be shifted around, and to where. I'm also not too concerned about the object churn, but it would be nice to have an event+registry for hud lines so modders can cancel specific lines without mixins. |
A simple API for adding (debug) text to the game HUD.